[#966] Airdrop endpoint security: rate limiting + cache headers#974
[#966] Airdrop endpoint security: rate limiting + cache headers#974realproject7 merged 3 commits intomainfrom
Conversation
…T endpoints Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
project7-interns
left a comment
project7-interns
left a comment
There was a problem hiding this comment.
Verdict: REQUEST CHANGES
Summary
The new rate-limiting utility is serverless-safe, but it is not concurrency-safe. Because it does a separate count query and then an insert, concurrent requests from the same IP can all observe the same pre-insert count and exceed the intended 5/min limit.
Findings
- [high]
checkRateLimit()is race-prone and can be bypassed under concurrent load. The current implementation doesSELECT count(*)for the window and then performs a separateINSERT. If multiple requests from the same IP hit at once, they can all seecount < 5before any of them insert, so more than 5 requests/min will be accepted. For a security control, this is a real enforcement bug.- File:
lib/rate-limit.ts:18 - Suggestion: Move this into a single atomic database operation, such as an RPC/function that both checks and records in one transaction, or a locking strategy that prevents concurrent over-admission.
- File:
Decision
Requesting changes because the rate limit can be bypassed by concurrency, so the main security control in this PR is not reliably enforced.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
project7-interns
left a comment
project7-interns
left a comment
There was a problem hiding this comment.
Verdict: REQUEST CHANGES
Summary
Moving the rate limit into a Supabase RPC is the right fix conceptually, but the new SQL function is not valid as written. It tries to apply FOR UPDATE to an aggregate count(*) query, which PostgreSQL does not allow, so the migration/function will fail or be unusable.
Findings
- [high]
check_rate_limit()usesSELECT count(*) ... FOR UPDATE, butFOR UPDATEcannot be applied to aggregate queries in PostgreSQL. That means the new atomic rate limiter does not actually deploy/work in its current form.- File:
supabase/migrations/00037_rate_limits.sql:34 - Suggestion: Use a transaction-safe locking strategy that locks rows or a per-key coordinator row before counting/inserting, or restructure the function so it does not rely on
FOR UPDATEover an aggregate result.
- File:
Decision
Requesting changes because the new database-side rate-limit function is invalid as written, so the main security control in this PR still is not ready.
… aggregate Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
project7-interns
left a comment
project7-interns
left a comment
There was a problem hiding this comment.
Verdict: APPROVE
Summary
The re-review update fixes the database-side rate limiter by replacing the invalid aggregate FOR UPDATE approach with a transaction-scoped advisory lock per key. That gives the POST airdrop endpoints a valid atomic rate-limit check while keeping the GET cache headers in place.
Findings
- No blocking findings.
Decision
Approving because the previous SQL validity issue in check_rate_limit() is resolved and the PR now satisfies the reviewed security hardening requirements for the airdrop endpoints. Checks visible to me were still pending at review time.
2 participants
Summary
rate_limitstable — serverless-safe (no in-memory state)lib/rate-limit.tsutility withcheckRateLimit()andgetClientIp()s-maxage+stale-while-revalidatefor CDN-level caching00037_rate_limits.sqlcreates table with index + purge triggerFixes #966
Test plan
🤖 Generated with Claude Code